Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove v1 implementation #179

Merged
merged 1 commit into from
Jun 23, 2022
Merged

Remove v1 implementation #179

merged 1 commit into from
Jun 23, 2022

Conversation

Wwwsylvia
Copy link
Member

Resolves #63

Signed-off-by: Lixia (Sylvia) Lei lixia_lei@outlook.com

@Wwwsylvia
Copy link
Member Author

Also removing the examples directory and the acceptance check since they are using the v1 implementation. We may want to have a new acceptance test for v2.

@shizhMSFT shizhMSFT requested review from shizhMSFT, jdolitsky, deitch, sajayantony and SteveLasker and removed request for shizhMSFT June 16, 2022 12:09
@shizhMSFT
Copy link
Contributor

The acceptance test is there for v1 since v1 does not have sufficient unit tests to catch regressions.

With tons of unit tests and testable examples, we don't need to maintain acceptance tests any more.

Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@deitch
Copy link
Contributor

deitch commented Jun 16, 2022

A few issues or questions.

  1. Should we be removing this without acceptance tests and examples? Maybe we should replace them in main before removing entirely? I saw we added a lot more testable examples. Are there enough to cover the existing and simple use cases? If so, then maybe we should link to it from the main page / README. Let's make it easy for people to get started.
  2. We link using the go package semver structure, i.e. oras.land/oras-go/v2; does that "do the right thing" when including the root dir of this package?
  3. Do we have a migrating log? Showing the differences between v1 and v2?

@shizhMSFT
Copy link
Contributor

Are there enough to cover the existing and simple use cases?

Yes, I'll create an issue for improving the doc to direct the users to the examples for push and pull.

does that "do the right thing" when including the root dir of this package?

That's actually how go module works. The module name for v2.x.x must ends with /v2 (and similarly /v3 for v3.0.0). Although the module name is oras.land/oras-go/v2, all source code SHOULD be placed at oras.land/oras-go.

BTW, users can import oras.land/oras-go/v2 and oras.land/oras-go at the same time. Hence, users can migrate from v1 to v2 iteratively.

Do we have a migrating log? Showing the differences between v1 and v2?

That's a good point. I will create an issue to track it.

@shizhMSFT
Copy link
Contributor

@shizhMSFT shizhMSFT mentioned this pull request Jun 23, 2022
Signed-off-by: Lixia (Sylvia) Lei <lixia_lei@outlook.com>
Copy link
Contributor

@deitch deitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing those two.

@shizhMSFT shizhMSFT merged commit 2210572 into oras-project:main Jun 23, 2022
@Wwwsylvia Wwwsylvia deleted the cleanup branch June 24, 2022 01:06
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove old implementation
3 participants